Alternatives for serving plugin #99

marefr · 2020-03-17T12:29:52Z

Suggestion to make it easier for a plugin developer to understand what interfaces they need to implement for a certain plugin type (datasource/app) and what interfaces we want to enforce a certain plugin to implement I started to think about using a "base struct" for default implementations. Struggled a lot with this "base struct" without being satisfied until I came to the conclusion that in general composition in favorable over inheritance.

In addition #96 suggested adding a new CheckDataSourceHealth to the existing QueryDataHandler to try and overcome the "weirdness" of having both a PluginConfig and a DataSourceConfig.

I think having a single interface that must be implemented per plugin type is desirable.

Current way of serving a plugin

https://github.com/grafana/google-sheets-datasource/blob/1e50d97f3bfd40870997039b17c0c8c901f8fbf8/pkg/main.go#L11-L27

Weakness with current approach is that there's nothing enforcing you to implement one interface, instead there's 4 different interfaces in backend.ServeOpts and it's probably not clear from a plugin developer perspective which ones I need/should implement.

Example 1

Must implement backend.DataSourcePlugin interface to be able to serve data source plugin.

package myplugin

import (
  "github.com/grafana/grafana-plugin-sdk-go/backend"
  "github.com/grafana/grafana-plugin-sdk-go/resource"
  "github.com/grafana/grafana-plugin-sdk-go/diagnostics"
)

type MyDataSource struct {
  CheckHealthHandler
  CallResourceHandler 
  logger log.Logger
}

func New(logger log.Logger, c backend.ConfigurePlugin) backend.DataSourcePlugin {
  c.MustRegister(<prometheus metrics>)

  // only implements QueryDataHandler by using composition and add default handlers for check health and call resource.
  return &MyDataSource{
    CheckHealthHandler: diagnostics.OKCheckHealthHandler(),
    CallResourceHandler: resource.NotFoundHandler(),
    logger: logger,
  }
}

func (ds *MyDataSource) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
  ...
}

package main

import (
  "github.com/grafana/grafana-plugin-sdk-go/backend"
)

func main() {
  backend.ServeDataSourcePlugin(myplugin.New)
}

Example 2

Using new datasource package with explicit datasource.Plugin interface. Must implement datasource.Plugin interface to be able to serve data source plugin.

package myplugin

import (
  "github.com/grafana/grafana-plugin-sdk-go/backend"
  "github.com/grafana/grafana-plugin-sdk-go/resource"
  "github.com/grafana/grafana-plugin-sdk-go/diagnostics"
)

type MyDataSource struct {
  datasource.CheckDataSourceHealthHandler
  datasource.CallDataSourceResourceHandler 
  logger log.Logger
}

func New(logger log.Logger, c backend.ConfigurePlugin) datasource.Plugin {
  c.MustRegister(<prometheus metrics>)

  // only implements QueryDataHandler by using composition and add default handlers for check health and call resource.
  return &MyDataSource{
    CheckDataSourceHealthHandler: datasource.NewCheckDataSourceHealthHandlerFunc(diagnostics.OKCheckHealthHandler()),
    CallDataSourceResourceHandler: datasource.NewCallDataSourceResourceHandlerFunc(resource.NotFoundHandler()),
    logger: logger,
  }
}

func (ds *MyDataSource) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
  ...
}

package main

import (
  "github.com/grafana/grafana-plugin-sdk-go/datasource"
)

func main() {
  datasource.Serve(myplugin.New)
}

bergquist · 2020-03-23T18:39:58Z

Great work! I like this. Make it ready for review? :)

kylebrandt · 2020-04-01T13:59:14Z

backend/serve.go

+	grpcplugin.Serve(pluginOpts)
+}
+
+type Plugin interface {


Comments please for these exported interfaces and the types that Follow

Yeah good point, but still a draft. So only consider the concept/examples

kylebrandt · 2020-04-01T14:01:22Z

backend/datasource/datasource.go

@@ -0,0 +1,85 @@
+package datasource


// Package datasource ....

marefr · 2020-04-01T14:07:40Z

The idea would be to stick with example 1 or example 2, not both. Same changes would be needed for both app and transform. Or keep it as is.

marefr · 2020-04-01T14:17:10Z

backend/datasource/datasource.go

+	OrgID            int64
+	DataSourceConfig backend.DataSourceConfig


Only OrgID and DataSourceConfig exposed here compared to regular CallResourceRequest which includes both PluginConfig and DataSourceConfig.

Helps not confusing plugin developer to use PluginConfig (currently only app plugins) when should be using DataSourceConfig.

marefr · 2020-04-01T14:19:21Z

backend/datasource/datasource.go

+type CallDataSourceResourceHandlerFunc func(ctx context.Context, req *CallDataSourceResourceRequest, sender backend.CallResourceResponseSender) error
+
+func (fn CallDataSourceResourceHandlerFunc) CallResource(ctx context.Context, req *backend.CallResourceRequest, sender backend.CallResourceResponseSender) error {
+	return fn(ctx, &CallDataSourceResourceRequest{


Not super found of this mapping back and forth, but removes the need of implementing yet another interface in resource and httpadapter packages.

marefr · 2020-04-01T14:20:22Z

backend/datasource/serve.go

+type Plugin interface {
+	CheckDataSourceHealthHandler
+	CallDataSourceResourceHandler
+	backend.QueryDataHandler


To be correct, this one should probably have a new interface in this package as well to only include DataSourceConfig in request and not PluginConfig.

Could be an interface similar to https://github.com/grafana/grafana/blob/master/pkg/tsdb/query_endpoint.go#L11, basically using similar DataSource model struct.

kylebrandt · 2020-04-01T16:14:27Z

Trying to think through using interfaces, and non-breaking upgrades. For example with the datasource.Plugin interface.

We release 1.0 with this interface
We release 1.1, adding a new RPC method, which means a new handler.
Plugin author fetches 1.1, which isn't supposed to have breaking changes

However, at step 3, wouldn't the plugin author get something like "MyDataSource is not a datasource.Plugin, missing QuerySloth(...) ...`

Edit: Looking at "Interfaces" section of https://about.sourcegraph.com/go/gophercon-2019-detecting-incompatible-api-changes

ryantxu · 2020-04-01T16:40:01Z

please please please please look at #105 and see if this is related.

Having an instance concept encapsulated by the API would be a really nice starting place

bergquist · 2020-05-05T06:02:55Z

@marefr lets close this?

Alternatives for serving plugin

c1d1fd6

marefr mentioned this pull request Mar 17, 2020

Adds healthcheck function to queryData endpoint #96

Closed

Merge remote-tracking branch 'origin/master' into serve_plugin_refactor

05a17f2

ryantxu mentioned this pull request Mar 24, 2020

Make "instance" a core SDK idea #105

Closed

marefr added 2 commits April 1, 2020 13:39

Exploratory: New datasource package for serving data source plugin

57a3311

Merge remote-tracking branch 'origin/master' into serve_plugin_refactor

613aa44

marefr requested review from kylebrandt, bergquist and ryantxu April 1, 2020 13:35

kylebrandt reviewed Apr 1, 2020

View reviewed changes

backend/datasource/datasource.go

@@ -0,0 +1,85 @@

package datasource

Copy link

Contributor

kylebrandt Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Package datasource ....

marefr marked this pull request as ready for review April 1, 2020 14:05

marefr commented Apr 1, 2020

View reviewed changes

marefr closed this May 5, 2020

marefr deleted the serve_plugin_refactor branch May 5, 2020 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternatives for serving plugin #99

Alternatives for serving plugin #99

marefr commented Mar 17, 2020 •

edited

Loading

bergquist commented Mar 23, 2020

kylebrandt Apr 1, 2020

marefr Apr 1, 2020

kylebrandt Apr 1, 2020

marefr commented Apr 1, 2020

marefr Apr 1, 2020 •

edited

Loading

marefr Apr 1, 2020

marefr Apr 1, 2020

marefr Apr 1, 2020

kylebrandt commented Apr 1, 2020 •

edited

Loading

ryantxu commented Apr 1, 2020

bergquist commented May 5, 2020

Alternatives for serving plugin #99

Alternatives for serving plugin #99

Conversation

marefr commented Mar 17, 2020 • edited Loading

Current way of serving a plugin

Example 1

Example 2

bergquist commented Mar 23, 2020

kylebrandt Apr 1, 2020

Choose a reason for hiding this comment

marefr Apr 1, 2020

Choose a reason for hiding this comment

kylebrandt Apr 1, 2020

Choose a reason for hiding this comment

marefr commented Apr 1, 2020

marefr Apr 1, 2020 • edited Loading

Choose a reason for hiding this comment

marefr Apr 1, 2020

Choose a reason for hiding this comment

marefr Apr 1, 2020

Choose a reason for hiding this comment

marefr Apr 1, 2020

Choose a reason for hiding this comment

kylebrandt commented Apr 1, 2020 • edited Loading

ryantxu commented Apr 1, 2020

bergquist commented May 5, 2020

marefr commented Mar 17, 2020 •

edited

Loading

marefr Apr 1, 2020 •

edited

Loading

kylebrandt commented Apr 1, 2020 •

edited

Loading